Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add events for row added and row updated #1101

Merged
merged 3 commits into from
Jul 18, 2024
Merged

feat: Add events for row added and row updated #1101

merged 3 commits into from
Jul 18, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented May 23, 2024

No description provided.

@come-nc come-nc added the 3. to review Waiting for reviews label May 23, 2024
@come-nc come-nc self-assigned this May 23, 2024
@come-nc come-nc requested a review from enjeck May 23, 2024 15:55
enjeck
enjeck reviewed May 26, 2024
View reviewed changes
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@come-nc Why do we need these events for?

@enjeck
Copy link
Contributor

enjeck commented May 27, 2024

Ah, I guess related to Workflows?

@come-nc
Copy link
Contributor Author

come-nc commented May 27, 2024

Ah, I guess related to Workflows?

Yes!

@enjeck enjeck requested a review from blizzz May 27, 2024 12:41
@enjeck enjeck requested a review from juliusknorr June 4, 2024 08:26
juliusknorr
juliusknorr reviewed Jun 7, 2024
View reviewed changes
lib/Event/RowAddedEvent.php Outdated
}

public function getRow(): Row2 {
return $this->row;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz What's your thought on just returning the entity directly here?

I'm unsure if we shall expose the internal row entity through events, rather expose specific getters or use a serializable event as #1103 mentioned

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have reservations about exposing the Entity. Turning this into a minified (and so serializable) model instance might could be a way. The Entity is can actually be json-serialized, that could be a base.

Then, in principle, whatever we are doing, we are de-facto exposing a Model to the outside, so setting standards as in an API, and have to keep them stable. That's generally fine, but would be a firstee at least in this app.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just read the note about OCP\EventDispatcher\IWebhookCompatibleEvent which is already in master meanwhile. But this is yet another jsonSerialize() style method. In a way i still tend to a public model class that itself is just json serializable and we return the output here. On the other hand, this can be already done with the DB Entity.

I also noticed that we are actually exposing the DB Entities already in other events that are also released already. Including Row2.

@juliusknorr
Copy link
Member

@come-nc To evaluate what we want to pass along in the event, could you share more insights on the use case you're covering with your workflow task?

@come-nc
Copy link
Contributor Author

come-nc commented Jun 10, 2024

@come-nc To evaluate what we want to pass along in the event, could you share more insights on the use case you're covering with your workflow task?

Usecases are multiple:

  • Have a workflow react to data inserted in a table (for instance a user is created in another application when a line is added to a table).
  • Have a workflow react to a modification of a row (typically a boolean column is checked to approve a vacation request or something, triggers a chain of events).

I feel like putting the whole row content information in the event is too much because it may be a lot of data, while putting only the id allows to fetch the data from the API if needed. Not entirely sure. Maybe content but capped at a given maximum length would make sense?

The event will need to be serialized by implementing upcomming OCP\EventDispatcher\IWebhookCompatibleEvent interface.

@blizzz blizzz force-pushed the feat/add-events-for-table-modification branch from 8b92f0d to a43232d Compare July 16, 2024 15:51
@blizzz
Copy link
Member

blizzz commented Jul 16, 2024

Fixed conflicts, and added two changes (cf. my comments #1101 (comment) and #1101 (comment)).

I added now a "Public" read-only Row model that is being attached to the Event.

I am lacking a concept on how the IWebhookCompatibleEvent is supposed to be used (left a 1:1 message with @marcelklehr), but it should contain the necessary data. The acting user is left out (previously discussed with Côme). The Column(s) can be derived from the change sets. Not the Cell, it's probably not necessary, and we actually don't have that information ourselves. Especially since changed data is already debated.

At the moment I send the row's values, and on update also the previous values. With Côme's statement:

I feel like putting the whole row content information in the event is too much because it may be a lot of data, while putting only the id allows to fetch the data from the API if needed. Not entirely sure. Maybe content but capped at a given maximum length would make sense?

It can be dropped indeed, return only the values that actually have changed (i.e. nothing on create and delete). Still can be a lot. Only the affected columns ids can be sent, too.

What I am having in mind is if process changes shall be followed. E.g. Something may change from "pending" to "open", and from "closed" to "open" and different things shall happen then. Again, I am missing context down the line, whether this is something reasonable, or not relevant here.

@blizzz blizzz self-assigned this Jul 16, 2024
@blizzz blizzz force-pushed the feat/add-events-for-table-modification branch from a43232d to 29098c9 Compare July 17, 2024 08:51
@blizzz blizzz requested review from marcelklehr and bigcat88 July 17, 2024 08:54
marcelklehr
marcelklehr approved these changes Jul 17, 2024
View reviewed changes
lib/Event/AbstractRowEvent.php

public function getWebhookSerializable(): array {
return $this->row->jsonSerialize();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesomesauce!

@blizzz blizzz force-pushed the feat/add-events-for-table-modification branch from 270ff93 to 92a2a12 Compare July 17, 2024 10:42
@blizzz blizzz linked an issue Jul 18, 2024 that may be closed by this pull request
Dispatch an Event when a value was changed #1103
Closed
come-nc and others added 3 commits July 18, 2024 14:21
@come-nc @juliusknorr
feat: Add events for row added and row updated
007ea40 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@blizzz @juliusknorr
feat(Events): webhook compatible Row events with public model
2a6a338 
- implements OCP\EventDispatcher\IWebhookCompatibleEvent
- add lib/Model/Public/Row as sort of API-like model for Tables Row data
- change exisiting events to use new abstract row event class

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz @juliusknorr
ci(psalm): update psalm to gain a bug fix
550a106 
otherwise we would see:
ERROR: UndefinedClass - lib/Event/AbstractRowEvent.php:12:22 - Class, interface or enum named OCP\EventDispatcher\IWebhookCompatibleEvent does not exist (see https://psalm.dev/019)
if (interface_exists(\OCP\EventDispatcher\IWebhookCompatibleEvent::class)) {

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@juliusknorr juliusknorr force-pushed the feat/add-events-for-table-modification branch from 92a2a12 to 550a106 Compare July 18, 2024 12:21
@juliusknorr juliusknorr added enhancement New feature or request workflow labels Jul 18, 2024
@juliusknorr juliusknorr added this to the v0.8.0 milestone Jul 18, 2024
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 18, 2024
@blizzz blizzz merged commit 54fb451 into main Jul 18, 2024
59 checks passed
@blizzz blizzz deleted the feat/add-events-for-table-modification branch July 18, 2024 15:14
@blizzz blizzz mentioned this pull request Nov 19, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enjeck enjeck enjeck left review comments

@juliusknorr juliusknorr juliusknorr approved these changes

@marcelklehr marcelklehr marcelklehr approved these changes

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner

@bigcat88 bigcat88 Awaiting requested review from bigcat88

Assignees

@blizzz blizzz

@come-nc come-nc

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request workflow
Projects
None yet
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.

Dispatch an Event when a value was changed
5 participants
@come-nc @enjeck @juliusknorr @blizzz @marcelklehr